ML-KEM assembly: AVX512F and AVX512VBMI#10981
Conversation
|
ab9e792 to
ba111bc
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10981
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: APPROVE
Findings: 1 total — 1 posted, 0 skipped
Posted findings
- [Medium] New AVX512 k2 noise path uses large unconditional stack buffers, unlike its AVX2 sibling —
wolfcrypt/src/wc_mlkem_poly.c:4754-4809
Review generated by Skoll via Claude/Codex
| * @param [out] rand Random number byte array (8 * PRF_RAND_SZ bytes). | ||
| * @param [in] seed Seed to generate random from. | ||
| */ | ||
| static void mlkem_get_noise_x8_eta3_avx512(byte* rand, byte* seed) |
There was a problem hiding this comment.
🟡 [Medium] New AVX512 k2 noise path uses large unconditional stack buffers, unlike its AVX2 sibling
💡 SUGGEST convention
mlkem_get_noise_k2_avx512 declares byte rand[8 * PRF_RAND_SZ] (PRF_RAND_SZ = 2*SHA3_256_BYTES = 272, so 2176 bytes) as a plain fixed stack array, and its helper mlkem_get_noise_x8_eta3_avx512 (and the eta2 helper) each declare word64 state[25 * 8] (1600 bytes) on the stack. The corresponding AVX2 sibling mlkem_get_noise_k2_avx2 instead respects WOLFSSL_SMALL_STACK via WC_DECLARE_VAR(rand, byte, 4 * PRF_RAND_SZ, 0) + WC_ALLOC_VAR_EX(...), and the new AVX512 matrix generators (mlkem_gen_matrix_k2/k3/k4_avx512) also use the XMALLOC small-stack pattern. So the new k2 noise path both doubles the buffer and drops the small-stack handling its own AVX2 twin provides, violating the project rule that large (>~100 byte) stack buffers use the WOLFSSL_SMALL_STACK pattern. Practical impact is low because these paths only execute on AVX-512-capable x86-64 (rarely a small-stack target), but it is an internal inconsistency worth aligning.
Suggestion: Use the WC_DECLARE_VAR / WC_ALLOC_VAR_EX pattern (as mlkem_get_noise_k2_avx2 does) for the rand and state buffers in mlkem_get_noise_k2_avx512 and the x8 helpers, returning MEMORY_E on allocation failure. Note k3/k4 avx512 mirror their avx2 siblings' plain-stack arrays, so this concern is specific to the k2 path.
New assembly for ML-KEM for Intel x64 machines that support AVX512 extensions. Update how much is added to the stack of others.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10981
Scan targets checked: wolfcrypt-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
No new issues found in the changed files. ✅
Description
New assembly for ML-KEM for Intel x64 machines that support AVX512 extensions.
Testing
Regression testing of ML-KEM.
Generator
https://github.com/wolfSSL/scripts/pull/638